Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[netlify] allow opting out of Image CDN #120

Merged
merged 5 commits into from
Jan 3, 2024

Conversation

Skn0tt
Copy link
Contributor

@Skn0tt Skn0tt commented Jan 2, 2024

Changes

As per #118, this adds an opt-out option for Image CDN.

Testing

Added new unit tests.

Docs

This is a new config option. We should probably mention it in the Astro integration docs.

Copy link

changeset-bot bot commented Jan 2, 2024

🦋 Changeset detected

Latest commit: 945ee59

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@astrojs/netlify Minor
@test/netlify-hosted-astro-project Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

*
* If disabled, Astro's built-in image optimization is run at build-time instead.
*
* @default enabled
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @default enabled
* @default {true}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're also using enabled/disabled wording for the comment above. Personally, I prefer the english wording here as opposed to code syntax - but it's a preference thing. Is there a guideline on this in the Astro docs? cc @sarah11918

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is JSDoc, which wants a value: https://jsdoc.app/tags-default

In this case, enabled isn't a valid value for this property

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point - gonna change this here and in the other comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in 945ee59

packages/netlify/src/index.ts Show resolved Hide resolved
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for addressing this so quickly!

@ematipico ematipico merged commit cf39b9d into withastro:main Jan 3, 2024
8 checks passed
@github-actions github-actions bot mentioned this pull request Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants